Skip to content

Make TAP devices usable in normal traffic scenarios #649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
guvenc merged 2 commits into from
Mar 11, 2025
Merged

Make TAP devices usable in normal traffic scenarios #649

guvenc merged 2 commits into from
Mar 11, 2025

Conversation

@guvenc
Copy link
Collaborator

@guvenc guvenc commented Feb 21, 2025 •
edited
Loading

Make TAP devices usable in normal traffic scenarios and not only pytest based test scenarios where MAC addresses and checksums can be ignored if not directly effecting the testing scenario.

@guvenc guvenc requested a review from a team as a code owner February 21, 2025 12:19
@github-actions github-actions bot added size/S enhancement New feature or request labels Feb 21, 2025
@PlagueCZ
Copy link
Contributor

PlagueCZ commented Feb 21, 2025

Hmm lookoing at what this does, it seems to me it would be much better to simply ask for config and then either call dp_get_pf_neigh_mac() or use our own.
Because this way we are calling "get neighbor" and then "oh what but in this special instance actually return something else and skip 99% of the code here).
There's even more space for commenting about it in dp_port.c where the call starts.

@guvenc
Copy link
Collaborator Author

guvenc commented Feb 21, 2025

Sure. I can change it that way. I am testing now whether it helps at all. If so, will improve the code.

@guvenc guvenc marked this pull request as draft February 21, 2025 12:39
@github-actions github-actions bot added size/M and removed size/S labels Feb 24, 2025
@guvenc guvenc force-pushed the feature/pf_tap_mac branch 2 times, most recently from 709b4e5 to 5d42f11 Compare February 26, 2025 11:48
@guvenc guvenc mentioned this pull request Feb 26, 2025
Closed
@guvenc guvenc force-pushed the feature/pf_tap_mac branch from 5d42f11 to 6e3f72c Compare March 10, 2025 10:58
@guvenc guvenc changed the title Use own mac as neighbour mac for PFs in TAP mode Make TAP devices usable in normal traffic scenarios Mar 10, 2025
@guvenc guvenc force-pushed the feature/pf_tap_mac branch 2 times, most recently from 5e2121f to 5ca89bb Compare March 10, 2025 11:35
@guvenc guvenc marked this pull request as ready for review March 10, 2025 11:39
@guvenc guvenc requested review from PlagueCZ and byteocean March 10, 2025 11:39
PlagueCZ
PlagueCZ reviewed Mar 10, 2025
View reviewed changes
Copy link
Contributor

@PlagueCZ PlagueCZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any of this PR was verified by pytest, please note that it is currently unable to detect some errors: #650
Maybe rebase and test on some older commit first.

@guvenc
Calculate nat packet checksum manually for TAP devices
7d5ff20 
Python Checksum checks seem to fail. Will ignore for the
time-being.

Signed-off-by: Guvenc Gulce <[email protected]>
@guvenc guvenc force-pushed the feature/pf_tap_mac branch from 5ca89bb to 49ce2d0 Compare March 10, 2025 17:49
@guvenc
Accept own MAC for PF neighbour in case of TAP device
b39a89b 
This is the FeBox case and it is ok to have the own MAC
as the neighbour MAC in case of TAP device mode.

Signed-off-by: Guvenc Gulce <[email protected]>
@guvenc guvenc force-pushed the feature/pf_tap_mac branch from 49ce2d0 to b39a89b Compare March 10, 2025 17:52
@guvenc guvenc requested a review from PlagueCZ March 10, 2025 17:54
PlagueCZ
PlagueCZ approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@PlagueCZ PlagueCZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now looks really nice. The OSC virtual lab can be adapted once I have something to work with.

src/dp_netlink.c
}
// If it is a tap device, we make an exception with own MAC address check. FeBOX case
if (dp_conf_is_tap_mode())
return DP_OK;
Copy link
Contributor

@PlagueCZ PlagueCZ Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh nice solution. So basically for TAP you accept the own mac here as you wanted and if we ever need a true neighbor mac it can be easily changed here, right?

Copy link
Collaborator Author

@guvenc guvenc Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would already accept true neighbor MAC. It just can not differentiate the "error case" anymore when it is in TAP mode. (Error case being "the router sending TAP device MAC as router MAC")

@guvenc guvenc merged commit 93d76b7 into main Mar 11, 2025
6 checks passed
@guvenc guvenc deleted the feature/pf_tap_mac branch March 11, 2025 07:18
@hardikdr hardikdr added this to Roadmap Jun 26, 2025
@hardikdr hardikdr moved this to Done in Roadmap Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PlagueCZ PlagueCZ PlagueCZ approved these changes

@byteocean byteocean Awaiting requested review from byteocean

Assignees

No one assigned

Labels

area/networking enhancement New feature or request size/M

Projects

Roadmap
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@guvenc @PlagueCZ @hardikdr